-
Notifications
You must be signed in to change notification settings - Fork 331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove unnecessary duplicated use of govuk-font #4267
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 32a6181 |
539c64d
to
45807e2
Compare
45807e2
to
158c944
Compare
packages/govuk-frontend/src/govuk/components/checkboxes/_index.scss
Outdated
Show resolved
Hide resolved
158c944
to
16d0085
Compare
.govuk-fieldset__legend--xl, | ||
.govuk-fieldset__legend--l, | ||
.govuk-fieldset__legend--m { | ||
@include govuk-typography-weight-bold; | ||
margin-bottom: govuk-spacing(3); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
packages/govuk-frontend/src/govuk/components/summary-list/_index.scss
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/warning-text/_index.scss
Outdated
Show resolved
Hide resolved
16d0085
to
e3118ff
Compare
This isn't necessary as we already have heading style modifiers for table captions. This does mean it's inconsistent with our docs that say they should correspond with heading typography classes. Suggest we should solve this holistically down the line.
e3118ff
to
32a6181
Compare
Think I'm happy with my final review 🙌 Shall we update the description to add Or split that out into another issue? I've not checked if we've caught them all |
@colinrotherham I've added it to the bottom of the description of this PR. I'm fairly confident we're using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking rather good 🙌
Can we have an updated file size saving figure? Unless it's bigger 😮
@colinrotherham Interestingly, the minified CSS hasn't changed but the map has increased by one kb 😮 I wonder how much of this is impacted by parallel work and rebasing. Not an analysis worth stressing over right now. Merging! |
Thanks @owenatgov We've added more Sass than we've removed I guess? All those source changes needs mapping They're are only downloaded when developer tools are open so it's fine by me |
What/Why
Remove instances of when our components are using
govuk-font
more than once when they don't need to.Fixes #4242. More details in the issue.
Notes
At time of writing, building
main
locally produces minified CSS of 123kb and a CSS map of 408kb. Building this branch locally produces minified CSS of 117kb and a CSS map of 397kb.The only 2 components that still have more than one use of
govuk-font
are the header and input component. Both of these are because of user agent styles that override the inherited font size, weight and font family. Specifically thebutton
element thatgovuk-header__menu-button
is using and theinput
forgovuk-input
.This change makes the following classes redundant:
govuk-panel__body
govuk-table__caption--s
Should we deprecate these classes?
This change additionally adds our default text colour to
govuk-summary-card
which was missed when we initially launched this.